Upstream and allow engines#2
Conversation
Fndsit/allow engines
agius
left a comment
agius
left a comment
There was a problem hiding this comment.
I think in the future it might be worth making two separate PRs: one to pull changes from upstream, and one for the changes we want to make. It just highlights the Trusted-specific changes a little better and makes them easier to talk about.
I'm skipping review for the upstream changes; presumably those have been checked by the Data Migrate team. I'm just looking at this commit with changes allowing our use of engines.
The code itself looks good. Two comments, but neither is really a blocker for deploying at Trusted. Before merging though, could we add a spec to spec/data_migrate/data_spec.rb or somewhere similar to test the functionality of the new code and prevent regressions?
| match_data = DataMigrate::DataMigrator.match(file) | ||
| versions << match_data[1].to_i if match_data | ||
| versions = Set.new | ||
| DataMigrate::DataMigrator.migrations_paths.each do |path| |
There was a problem hiding this comment.
It looks like this might be changing the list from an expanded path to a relative path. It might be an issue if running the data_migrate command from somewhere other than Rails.root ? Pretty edge case, would not consider this a blocker for deployment, but maybe something the upstream folks will be concerned about 😸
| versions << match_data[1].to_i if match_data | ||
| end | ||
| end | ||
| versions |
There was a problem hiding this comment.
Set and Array have pretty different methods available. If it doesn't break anything, I think changing to Set here is an improvement since it removes duplicates. But if we wanted to minimize changes to method signatures, we could put a .to_a here so the method returns the same type as it did before.
3 participants
This is an update which fixes the failing tests in ilyakatz#225 as well as ensures that the data rake tasks run successfully when there are multiple migration paths due to engines also using data migrations.
another alternative here is to install the migrations into the host application, which is how i believe schema migrations work. open to thoughts on strategy here!